-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial testing support #44
Conversation
public String getDisplayName() { | ||
return displayName; | ||
} | ||
public String getDisplayName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra leading spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All other files are indented with 4 spaces per tab, this brings this file into line with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, kk. Thought it was tab, plus space.
👍
public String getUrlName() { | ||
return urlName; | ||
} | ||
public String getUrlName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leading spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto above.
@@ -353,17 +339,14 @@ private boolean performForApplication(Run<?, ?> build, FilePath workspace, EnvVa | |||
EnvAction envData = new EnvAction(); | |||
build.addAction(envData); | |||
|
|||
if (envData != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer guarding against a null envData? Actually, will it ever not be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's safe because Java guarantees that the new operator will never return null.
These tests look awesome. |
Thanks :) The code style is the one I used on my last, and only, substantial java project, where it was heavily enforced - so now I'm completely institutionalised. It also happens to be the default code style used by the intelliJ IDE. If you disagree with it I can change it though, I have no strong feelings about the choice of code style. |
Don't disagree on the convention, just curious. I'm mostly concerned with consistency more than anything. Thx! |
Hi @SimonStJG thank you so much for these contributions. Sorry to be an arse, but can I get you to fix your PR as master has moved on a bit since you did this. I'd love to get this in for the next release if possible. |
Hi @mezpahlan I'd forgotten all about this :) Sure will find some time this weekend to fix it. |
* Remove pointless null checks * Use Jenkins.getInstance as Hudson.getIntance is deprecated * Use try-with-resources where possible
@mezpahlan All sorted now I hope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these. Looks fab!
I've edited the title for the PR as this is clearly more than just retrofit tests! :) |
Tests which I wrote as part of PR 41.